-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Locking by spending and creating UTXOs #121
Conversation
Signed-off-by: Jim Zhang <[email protected]>
commit 97a67ac88dba76eabb78f844be83a94d147939bf Author: Jim Zhang <[email protected]> Date: Wed Jan 8 16:46:48 2025 -0500 update locking for nullifier based tokens Signed-off-by: Jim Zhang <[email protected]> commit dc5de95cc538736fbb2d4156ad952f51020a60a2 Author: Jim Zhang <[email protected]> Date: Wed Jan 8 16:45:38 2025 -0500 spend inputs when locking Signed-off-by: Jim Zhang <[email protected]> commit 07b42e72db8c94c81ec87c11c1b419507b858550 Merge: 7751a58 cb39891 Author: Jim Zhang <[email protected]> Date: Mon Jan 6 11:18:22 2025 -0500 Merge branch 'main' into locking commit 7751a5808ddcbf54db643d9f8c969d86b84677a3 Author: Jim Zhang <[email protected]> Date: Sun Jan 5 21:50:45 2025 -0500 --amend commit 5d1314b1262531a8cc4af9d084e5016e0984d2a8 Author: Jim Zhang <[email protected]> Date: Fri Jan 3 17:17:31 2025 -0500 enhance locking with outputs Signed-off-by: Jim Zhang <[email protected]> Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
6706e9e
to
4725fba
Compare
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
) { | ||
revert UTXOAlreadyLocked(sortedInputs[i]); | ||
} | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like all these "if" statements could be nested into a big if (inputsLocked) {} else {}
block. It's almost entirely different checks for locked inputs vs unlocked ones.
solidity/contracts/lib/zeto_base.sol
Outdated
@@ -118,4 +190,67 @@ abstract contract ZetoBase is IZetoBase, ZetoCommon { | |||
} | |||
emit UTXOMint(utxos, msg.sender, data); | |||
} | |||
// Locks the UTXOs so that they can only be spent by submitting the appropriate | |||
// proof from the Eth account designated as the "delegate". This function | |||
// should be called by escrow contracts that will use uploaded proofs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small note, but "lock" wouldn't actually be called by the escrow contract - it would have to be called by the account owner establishing the the escrow contract.
solidity/contracts/lib/zeto_base.sol
Outdated
delegates[lockedOutputs[i]] != address(0) && | ||
delegates[lockedOutputs[i]] != msg.sender | ||
) { | ||
revert UTXOAlreadyLocked(lockedOutputs[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be NotLockDelegate
?
) { | ||
revert UTXOAlreadyLocked(lockedOutputs[i]); | ||
} | ||
delegates[lockedOutputs[i]] = delegate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we set _lockedUtxos
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see that it's in another method, processInputsAndOutputs
. It's slightly confusing that the setup of the locked states is split up. Maybe this method just needs a different name?
@@ -59,6 +69,8 @@ abstract contract ZetoNullifier is IZetoBase, ZetoCommon { | |||
revert UTXODuplicate(sortedInputs[i]); | |||
} | |||
if (_nullifiers[sortedInputs[i]] == true) { | |||
console.log("nullifier"); | |||
console.log(sortedInputs[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these stay here?
solidity/contracts/test/escrow2.sol
Outdated
|
||
/// @title A sample on-chain implementation of an escrow contract using Zeto tokens | ||
/// @author Kaleido, Inc. | ||
/// @dev Implements escrow based payment flows with Zeto_Anon tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is with Zeto_AnonNullifier
I like the direction here, and I think it makes sense. Since it's in draft state, I've mainly reviewed the documentation and looked through the contracts at a high level. I haven't yet done a detailed review of the contracts and circuits. |
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
doc-site/docs/images/overview.jpg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't spot the difference here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the spelling of "woner" vs. "owner" in the label on the bottom 😄
if ( | ||
_lockedUtxos[sortedInputs[i]] == UTXOStatus.SPENT || | ||
_utxos[sortedInputs[i]] == UTXOStatus.SPENT | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the logic, it feels like:
we allow the same UTXO hash to exist in both the regular and locked UTXO tracking map.
but once one of the UTXO with that hash is spent, the other UTXO will not be spendable.
@jimthematrix did I miss a uniqueness check somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't allow the same UTXO to exist in both maps at the same time. if you think that's a possibility due to a hole in the current validation logic, please provide the details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the first read, it wasn't clear to me the whether someone can use the lock function to generate same utxos that are in different maps. I added a test to prove that's not possible: 8ced077
Co-authored-by: Chengxuan Xing <[email protected]> Signed-off-by: jimthematrix <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
// With the above steps, we demonstrated that the nullifiers | ||
// are securely bound to the input commitments. Now we need to | ||
// demonstrate that the input commitments belong to the Sparse | ||
// Merkle Tree with the root `root`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure these comments are up to date, I don't see any above steps
for the rest of the comments, it's unclear what's the relationship between delegates & root
zkp/circuits/basetokens/anon_nullifier__transferLocked_base.circom
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. Usage of node values for enhance circuit security for locked UTXOs is great.
Signed-off-by: Jim Zhang <[email protected]>
per discussions in #119, re-implementing state locking with the following functions:
lock(inputs, outputs, lockedOutputs, delegate)
will spend the unlocked inputs and create a combination of unlocked inputs and locked inputs. Locked UTXOs must be spent by submitting the proper proof from the delegate account. The delegate account with be an EOA or a contractdelegateLock(utxos, newDelegate)
allows the current delegate to move the lock to a new delegatetransfer(inputs, outputs, proof)
is updated to spend only unlocked UTXOstransferLocked(inputs, outputs, proof)
is added to spend locked UTXOs